Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sumcheck: contractHandler.js #1478

Closed
wants to merge 3 commits into from
Closed

Conversation

varun-doshi
Copy link

@varun-doshi varun-doshi commented Oct 10, 2024

Description

Concise description of proposed changes

Closes #1465

Testing

Explain the quality checks that have been done on the code changes

Additional Information

Your ENS/address:
0x00655ABFA67C746F1b4F6DDD86Cf762b0D36DE07

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling for contract-related requests, enhancing clarity in error messages.
    • Ensured consistent representation of addresses in log messages and error reporting.
  • Chores

    • Refined logging practices for better clarity regarding contract bytecode and precompile absence.
    • Introduced a new utility function for address formatting to ensure checksum compliance.

Copy link

changeset-bot bot commented Oct 10, 2024

⚠️ No Changeset found

Latest commit: df6a9f6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Oct 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
tevm-monorepo-tevm ⬜️ Ignored (Inspect) Oct 10, 2024 8:05pm

Copy link
Contributor

coderabbitai bot commented Oct 10, 2024

Walkthrough

The changes involve modifications to the contractHandler function in packages/actions/src/Contract/contractHandler.js. The updates focus on how the params.to address is processed through the new toChecksumAddress utility, ensuring it is consistently formatted as a checksum address. This includes enhancements to error messages related to contract existence and encoding issues, specifically improving the clarity of the InvalidRequestError. A new utility function for checksumming addresses has also been introduced.

Changes

File Path Change Summary
packages/actions/src/Contract/contractHandler.js Incorporated toChecksumAddress for address handling in logs and error messages; refined error handling.
packages/actions/src/utils/toChecksumAddress.js Introduced toChecksumAddress utility function for validating and formatting addresses.

Assessment against linked issues

Objective Addressed Explanation
Addresses in logs are not checksummed (#1465)

Possibly related PRs

  • 👷 Address-prod #1447: The changes in this PR involve error handling and documentation related to Ethereum addresses, which may relate to the improvements in error messaging and address handling in the main PR.

Poem

🐇 In the code where contracts dwell,
A string conversion rings a bell.
Errors clearer, logs refined,
A rabbit's joy in each line aligned!
Hops of clarity, leaps of cheer,
In every log, the address is clear! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
packages/actions/src/Contract/contractHandler.js (3)

55-57: Approved: Improved address logging

The change to convert params.to to a string in the debug log statement is a good improvement. It ensures consistent representation of the address in the log output.

For consistency, consider applying the same toString() conversion to the contract and precompile properties in the debug object as well.


65-66: Approved: Enhanced error message clarity

The change to convert params.to to a string in the error message is a good improvement. It ensures consistent representation of the address in error outputs.

Consider enhancing the error handling further by including more context in the error message. For example:

message: `Contract at address ${params.to.toString()} does not exist. Please check the address and ensure it's deployed.`,

This provides more guidance to users encountering this error.


Line range hint 1-168: Suggestion: Implement address checksumming

While the changes improve the string representation of addresses in logs and error messages, they don't explicitly implement checksumming as mentioned in the PR objectives (issue #1465). Consider using a checksumming function to ensure addresses are properly formatted.

You could implement this by creating a utility function:

import { getAddress } from '@ethersproject/address';

function toChecksumAddress(address) {
    return getAddress(address);
}

Then use this function when converting addresses to strings:

to: toChecksumAddress(params.to),

This would ensure that addresses are consistently checksummed throughout the codebase.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 95b7b03 and 4bc25cb.

📒 Files selected for processing (1)
  • packages/actions/src/Contract/contractHandler.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/actions/src/Contract/contractHandler.js (1)

Line range hint 1-168: Summary: Improved address handling, but checksumming not fully implemented

The changes in this PR improve the handling of addresses in logs and error messages by consistently converting them to strings. This enhances the clarity of debug outputs and error messages. However, the primary objective of ensuring addresses are checksummed (as per issue #1465) is not fully addressed.

Key points:

  1. Address string conversion is now consistent in logs and error messages.
  2. Error messages provide clearer context about non-existent contracts.
  3. The core functionality of contractHandler remains intact.

To fully meet the PR objectives, consider implementing explicit checksumming of addresses as suggested in the previous comment.

To ensure no unintended changes were introduced, please run the existing test suite for this module.

Copy link
Collaborator

@roninjin10 roninjin10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr is just calling toString on a string so it is not doing anything.

@varun-doshi
Copy link
Author

Wrong commit was pushed
Have updated the code

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
packages/actions/src/utils/toChecksumAddress.js (2)

19-20: LGTM: Keccak-256 hashing is correctly implemented.

The use of keccak256 and stringToHex from @tevm/utils is appropriate, and the hashing process follows the EIP-55 specification.

Consider using const instead of let for the hash variable as it's not reassigned:

const hash = keccak256(stringToHex(addressLower));
🧰 Tools
🪛 Biome

[error] 20-20: This let declares a variable that is only assigned once.

'hash' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)


1-41: Overall assessment: Function implements EIP-55 correctly with room for improvements.

The toChecksumAddress function successfully implements the EIP-55 checksum address algorithm, addressing the issue mentioned in PR #1478 and the linked issue #1465. It ensures that addresses are properly checksummed, which was the main objective of this PR.

Key points:

  1. The core logic correctly implements the checksum algorithm.
  2. Input validation can be enhanced for better robustness.
  3. Some optimizations can be made for efficiency and type safety.

These improvements will further enhance the reliability and performance of the function without altering its core functionality.

Consider adding unit tests to verify the function's behavior with various input types and edge cases. This will ensure the function remains correct as the codebase evolves.

🧰 Tools
🪛 Biome

[error] 20-20: This let declares a variable that is only assigned once.

'hash' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)


[error] 29-29: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4bc25cb and c8409f8.

📒 Files selected for processing (2)
  • packages/actions/src/Contract/contractHandler.js (3 hunks)
  • packages/actions/src/utils/toChecksumAddress.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/actions/src/Contract/contractHandler.js
🧰 Additional context used
🪛 Biome
packages/actions/src/utils/toChecksumAddress.js

[error] 20-20: This let declares a variable that is only assigned once.

'hash' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)


[error] 29-29: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)

🔇 Additional comments (2)
packages/actions/src/utils/toChecksumAddress.js (2)

1-7: LGTM: Imports and function signature are well-defined.

The imports from '@tevm/utils' are appropriate for the function's purpose. The use of JSDoc comments for parameter and return type documentation is a good practice. The function signature with type assertion for the parameter enhances type safety.


38-41: LGTM: Function return is correct.

The return statement is straightforward and correctly returns the constructed checksum address.

Comment on lines 9 to 17
if (!address) {
throw new Error('Invalid address provided');
}


let addressLower=address.toLowerCase().replace(/^0x/, '');
if(!addressLower){
addressLower='0x';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance input validation for robustness.

While the current input validation catches falsy values, it could be more robust:

  1. Check if the input is a string.
  2. Validate the address format (correct length, valid hexadecimal characters).
  3. Consider using a regular expression for format validation.

Here's a suggested improvement:

if (typeof address !== 'string' || !/^(0x)?[0-9a-fA-F]{40}$/.test(address)) {
  throw new Error('Invalid Ethereum address format');
}

let addressLower = address.toLowerCase().replace(/^0x/, '');
if (!addressLower) {
  addressLower = '0x';
}

This change ensures that the input is a string and matches the expected Ethereum address format before processing.

Comment on lines 23 to 36
// Iterate over each character of the address and the corresponding hash
let checksumAddress = '0x';
for (let i = 0; i < addressLower.length; i++) {
if(hash[i]){

// @ts-ignore
if (parseInt(hash[i], 16) >= 8) {
// @ts-ignore
checksumAddress += addressLower[i].toUpperCase();
} else {
checksumAddress += addressLower[i];
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve checksum address construction and type handling.

The logic for constructing the checksum address is correct, but there are a few areas for improvement:

  1. Replace @ts-ignore with proper type handling.
  2. Use Number.parseInt instead of parseInt as suggested by the static analysis.
  3. Consider using Buffer for more efficient byte manipulation.

Here's a suggested improvement:

const checksumAddress = '0x' + Buffer.from(addressLower)
  .map((char, i) => {
    const charCode = char < 97 ? char - 48 : char - 87;
    return Number.parseInt(hash[i], 16) > 7 ? char.toString().toUpperCase() : char.toString();
  })
  .join('');

This change eliminates the need for @ts-ignore, uses Number.parseInt, and potentially improves performance by using Buffer.

🧰 Tools
🪛 Biome

[error] 29-29: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
packages/actions/src/utils/toChecksumAddress.js (2)

3-7: Enhance JSDoc comment for better documentation.

The current JSDoc comment provides basic information about the function's parameter and return type. However, it could be more descriptive to improve code documentation.

Consider expanding the JSDoc comment as follows:

/**
 * Converts an Ethereum address to its checksum format.
 * @param {string} address - The Ethereum address to convert (with or without '0x' prefix).
 * @returns {string} The checksummed Ethereum address.
 * @throws {Error} If the input is not a valid Ethereum address format.
 */

This enhanced comment provides more context about the function's purpose and behavior.


20-20: Use const for variables that are not reassigned.

The static analysis tool correctly points out that the hash variable is only assigned once and never reassigned.

Change the declaration of hash from let to const:

- let hash = keccak256(stringToHex(addressLower));
+ const hash = keccak256(stringToHex(addressLower));

This change improves code clarity by indicating that hash is not meant to be reassigned.

🧰 Tools
🪛 Biome

[error] 20-20: This let declares a variable that is only assigned once.

'hash' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c8409f8 and df6a9f6.

📒 Files selected for processing (1)
  • packages/actions/src/utils/toChecksumAddress.js (1 hunks)
🧰 Additional context used
🪛 Biome
packages/actions/src/utils/toChecksumAddress.js

[error] 23-37: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 31-35: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)


[error] 33-35: This else clause can be omitted because previous branches break early.

(lint/style/noUselessElse)


[error] 20-20: This let declares a variable that is only assigned once.

'hash' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🔇 Additional comments (1)
packages/actions/src/utils/toChecksumAddress.js (1)

9-11: LGTM: Robust input validation implemented.

The input validation has been significantly improved:

  1. It checks if the input is a string.
  2. It validates the address format using a regular expression.
  3. It throws a clear error message for invalid inputs.

This implementation addresses the suggestions from the previous review and enhances the function's robustness.

Comment on lines +23 to +37
const checksumAddress = '0x' + Array.from(addressLower)
.map((char, i) => {
const charCode = char.charCodeAt(0);
// @ts-ignore
const shouldUpperCase = Number.parseInt(hash[i], 16) > 7;

if (charCode >= 48 && charCode <= 57) { // '0'-'9'
return char; // Numbers remain unchanged
} else if (charCode >= 97 && charCode <= 102) { // 'a'-'f'
return shouldUpperCase ? char.toUpperCase() : char;
} else {
throw new Error(`Invalid character ${char} in address.`);
}
})
.join('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refine checksum address construction for better readability and type safety.

The checksum address construction logic is correct, but there are a few areas for improvement:

  1. Use template literals as suggested by the static analysis tool.
  2. Remove the @ts-ignore comment by using proper typing.
  3. Consider using Buffer for more efficient byte manipulation, as suggested in a previous review.

Here's a suggested improvement:

const checksumAddress = `0x${Buffer.from(addressLower).map((char, i) => {
  const charCode = char < 97 ? char - 48 : char - 87;
  return parseInt(hash[i * 2 + 2], 16) > 7 ? char.toString(16).toUpperCase() : char.toString(16);
}).join('')}`;

This change:

  • Uses template literals for better readability.
  • Eliminates the need for @ts-ignore by using Buffer.
  • Potentially improves performance by using Buffer.
  • Simplifies the logic by using the numeric values of characters directly.

Note: If you decide not to use Buffer, at least consider removing the @ts-ignore comment and properly typing the hash variable:

const hash: string = keccak256(stringToHex(addressLower));

This will improve type safety without needing the comment.

🧰 Tools
🪛 Biome

[error] 23-37: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 31-35: This else clause can be omitted because previous branches break early.

Unsafe fix: Omit the else clause.

(lint/style/noUselessElse)


[error] 33-35: This else clause can be omitted because previous branches break early.

(lint/style/noUselessElse)

Comment on lines +14 to +17
let addressLower=address.toLowerCase().replace(/^0x/, '');
if(!addressLower){
addressLower='0x';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider removing unnecessary empty string check.

The input validation on lines 9-11 ensures that the address is a valid Ethereum address format. Therefore, the check for an empty string after preprocessing (lines 15-17) is unnecessary.

Consider simplifying this part of the code:

 let addressLower = address.toLowerCase().replace(/^0x/, '');
-if (!addressLower) {
-  addressLower = '0x';
-}

This change will make the code more concise without affecting its functionality, as the input validation guarantees a non-empty valid address.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let addressLower=address.toLowerCase().replace(/^0x/, '');
if(!addressLower){
addressLower='0x';
}
let addressLower=address.toLowerCase().replace(/^0x/, '');

Copy link
Collaborator

@roninjin10 roninjin10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is still not correct. It's not addressing any of the major places we care about this (moreso in return types not log messages) and it's using a bespoketoChecksumAddress rather than the built in checksumming that already exists in the tevm/address package

join telegram if you want more guidance

@roninjin10 roninjin10 closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Addresses in logs are not checksummed
2 participants